Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

docs: warn users about non-ASCII paths on build #16735

Closed
wants to merge 1 commit into from
Closed

docs: warn users about non-ASCII paths on build #16735

wants to merge 1 commit into from

Conversation

mmarchini
Copy link
Contributor

The build breaks if there's a non-ASCII character on the path to the building
directory.

Ref: #16278
Ref: #14336

Checklist
Affected core subsystem(s)

doc

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Nov 4, 2017
@bnoordhuis
Copy link
Member

#14336 was ostensibly addressed by #16047 although I won't vouch it always work everywhere now, see discussion in the PR.

@mmarchini
Copy link
Contributor Author

mmarchini commented Nov 6, 2017 •
edited
Loading

Apparently, #16047 addresses only make lint, but make -j4 is also crashing. Just tested on Ubuntu 16.04 and it crashes if there's a Unicode character on the path.

LD_LIBRARY_PATH=/home/matheus/workspace/sthima/tmp/joão/node/out/Release/lib.host:/home/matheus/workspace/sthima/tmp/joão/node/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../deps/v8/src/inspector; mkdir -p /home/matheus/workspace/sthima/tmp/joão/node/out/Release/obj/gen/src/inspector/protocol /home/matheus/workspace/sthima/tmp/joão/node/out/Release/obj/gen/include/inspector; python ../../third_party/inspector_protocol/CodeGenerator.py --jinja_dir ../../third_party --output_base "/home/matheus/workspace/sthima/tmp/joão/node/out/Release/obj/gen/src/inspector" --config inspector_protocol_config.json
Failed to parse config file: 'ascii' codec can't decode byte 0xc3 in position 37: ordinal not in range(128)

deps/v8/src/inspector/protocol_generated_sources.target.mk:16: recipe for target '1ddbb19584026854c12fe832a3659fb93afcac00.intermediate' failed
make[1]: *** [1ddbb19584026854c12fe832a3659fb93afcac00.intermediate] Error 1
rm 1ddbb19584026854c12fe832a3659fb93afcac00.intermediate
Makefile:81: recipe for target 'node' failed
make: *** [node] Error 2

Path used was: /home/matheus/workspace/sthima/tmp/joão/node.

@bnoordhuis
Copy link
Member

Ah, that's been fixed upstream (link) but hasn't made its way downstream yet.

@mmarchini
Copy link
Contributor Author

Tested the upstream fix on Ubuntu 16.04 and Windows 10. Ubuntu is working, but Windows is still breaking:

image
(https://gist.github.com/anonymous/875aac0ab3e4dfa219854b691f8d3431)

Probably unrelated to #14336 as it seems to be a Windows-only problem with non-ASCII characters, I'll open a new issue for this.

@mmarchini mmarchini mentioned this pull request Nov 16, 2017
Closed
addaleax
addaleax reviewed Dec 10, 2017
View reviewed changes
BUILDING.md Outdated
@@ -97,7 +97,8 @@ More Developer Tools...`. This step will install `clang`, `clang++`, and
* After building, you may want to setup [firewall rules](tools/macosx-firewall.sh)
to avoid popups asking to accept incoming network connections when running tests:

If the path to your build directory contains a space, the build will likely fail.
If the path to your build directory contains a space or a non-ASCII character, the
build will likely fail.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not put this into the unix section, experiences from recent Code & Learn events have shown that we are not usually that far away from this working.

The only issue I know of is fixed by v8/v8@744b49e, which comes with the V8 6.4 upgrade (#17489).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense.

PR updated with the warning removed from the Unix section, keeping it only on the Windows section

docs: warn users about non-ASCII paths on build
0e90f1a 
The build breaks if there's a non-ASCII character on the path to the building
directory.

Ref: nodejs#16278
Ref: nodejs#14336
@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 17, 2018
@joyeecheung
Copy link
Member

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/98/

@joyeecheung
Copy link
Member

Landed in c134ff2, thanks!

joyeecheung pushed a commit that referenced this pull request Jan 18, 2018
@joyeecheung
doc: warn users about non-ASCII paths on build
c134ff2 
The build breaks if there's a non-ASCII character on
the path to the building directory.

PR-URL: #16735
Refs: #16278
Refs: #14336
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2018
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
@evanlucas
doc: warn users about non-ASCII paths on build
35ef067 
The build breaks if there's a non-ASCII character on
the path to the building directory.

PR-URL: #16735
Refs: #16278
Refs: #14336
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@evanlucas evanlucas mentioned this pull request Jan 30, 2018
Merged
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
@evanlucas
doc: warn users about non-ASCII paths on build
1ef8f4e 
The build breaks if there's a non-ASCII character on
the path to the building directory.

PR-URL: #16735
Refs: #16278
Refs: #14336
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
@MylesBorins
doc: warn users about non-ASCII paths on build
5f1803b 
The build breaks if there's a non-ASCII character on
the path to the building directory.

PR-URL: #16735
Refs: #16278
Refs: #14336
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
@MylesBorins
doc: warn users about non-ASCII paths on build
2e76df5 
The build breaks if there's a non-ASCII character on
the path to the building directory.

PR-URL: #16735
Refs: #16278
Refs: #14336
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
This was referenced Feb 27, 2018
Closed
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@MayaLekova
doc: warn users about non-ASCII paths on build
f1bb2a4 
The build breaks if there's a non-ASCII character on
the path to the building directory.

PR-URL: nodejs#16735
Refs: nodejs#16278
Refs: nodejs#14336
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@lundibundi lundibundi mentioned this pull request Aug 25, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@gibfahn gibfahn gibfahn approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mmarchini @bnoordhuis @joyeecheung @jasnell @addaleax @cjihrig @gibfahn @MylesBorins @nodejs-github-bot